Skip to content

[realppl 5] map,string,timestamp and mirroring semantics #14851

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 19, 2025
Merged

[realppl 5] map,string,timestamp and mirroring semantics #14851

merged 1 commit into from
Sep 19, 2025

Conversation

wu-hui
Copy link
Contributor

@wu-hui wu-hui commented May 13, 2025

No description provided.

@gemini-code-assist Gemini Code Assist
Copy link
Contributor

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@wu-hui wu-hui force-pushed the wuandy/RealPpl_5 branch from 89aa149 to 16f3645 Compare May 14, 2025 23:38
@wu-hui wu-hui force-pushed the wuandy/RealPpl_4 branch from 3cc1d68 to 1d3466e Compare May 14, 2025 23:38
@wu-hui wu-hui force-pushed the wuandy/RealPpl_5 branch from 16f3645 to f0b8f3e Compare May 27, 2025 06:49
@wu-hui wu-hui force-pushed the wuandy/RealPpl_4 branch from 1d3466e to 240fe5f Compare May 27, 2025 06:49
tom-andersen
tom-andersen reviewed Jun 2, 2025
View reviewed changes
Firestore/core/test/unit/core/expressions/timestamp_test.cc
ReturnsNull());
}

TEST_F(TimestampAddTest, TimestampAddNullTimeUnitReturnsNull) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an error in backend AFAIK. The expectation is that unit is a string constant. It cannot be evaluated per document. Nor can it be a null.

@wu-hui wu-hui force-pushed the wuandy/RealPpl_5 branch from f0b8f3e to d7e061b Compare June 3, 2025 05:43
@wu-hui wu-hui force-pushed the wuandy/RealPpl_4 branch from 240fe5f to 60add0f Compare June 3, 2025 05:43
@wu-hui wu-hui force-pushed the wuandy/RealPpl_5 branch from d7e061b to d32e2ba Compare June 30, 2025 18:37
@wu-hui wu-hui force-pushed the wuandy/RealPpl_4 branch from 60add0f to 0145550 Compare June 30, 2025 18:37
@wu-hui wu-hui force-pushed the wuandy/RealPpl_5 branch from d32e2ba to 3a7418a Compare July 3, 2025 17:38
@wu-hui wu-hui force-pushed the wuandy/RealPpl_4 branch 2 times, most recently from 51c65a8 to 7941d68 Compare July 7, 2025 17:19
@wu-hui wu-hui force-pushed the wuandy/RealPpl_5 branch from 3a7418a to 3e452b5 Compare July 7, 2025 17:19
@wu-hui wu-hui requested a review from ehsannas September 10, 2025 19:06
@wu-hui wu-hui force-pushed the wuandy/RealPpl_4 branch 2 times, most recently from 0feb290 to aa284d1 Compare September 12, 2025 18:25
@wu-hui wu-hui force-pushed the wuandy/RealPpl_5 branch 2 times, most recently from 3f5c55e to 6c0a698 Compare September 12, 2025 18:30
@wu-hui wu-hui force-pushed the wuandy/RealPpl_4 branch 2 times, most recently from 6294868 to cd9cdfc Compare September 15, 2025 15:40
@wu-hui wu-hui force-pushed the wuandy/RealPpl_5 branch 2 times, most recently from 9051e4f to e67b0ac Compare September 15, 2025 15:42
Base automatically changed from wuandy/RealPpl_4 to cheryllin/ppl September 17, 2025 15:04
ehsannas
ehsannas approved these changes Sep 18, 2025
View reviewed changes
Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with utf-8 validation (the ProcessUtf8 function). So I ended up googling a lot of it. It looks good, and the unit test coverage for it is good.

Firestore/core/src/model/value_util.cc Outdated
const google_firestore_v1_MapValue& map_value = value.map_value;

// MapValues in iOS are always stored in sorted order.
auto found = std::equal_range(map_value.fields,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized: It looks like equal_range was introduced in c++20 (https://en.cppreference.com/w/cpp/algorithm/equal_range.html). The iOS SDK uses c++14 right now (and will upgrade to c++17 next IIRC)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find. I think this is a mistake completely. Fixed.

@ehsannas ehsannas assigned wu-hui and unassigned ehsannas Sep 18, 2025
@wu-hui wu-hui merged commit 2c5f607 into cheryllin/ppl Sep 19, 2025
8 of 16 checks passed
@wu-hui wu-hui deleted the wuandy/RealPpl_5 branch September 19, 2025 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tom-andersen tom-andersen tom-andersen left review comments

@ehsannas ehsannas ehsannas approved these changes

Assignees

@wu-hui wu-hui

Labels
api: firestore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wu-hui @google-oss-bot @ehsannas @tom-andersen